Improve state and config changes in ec2_instance #550
Conversation
DO NOT MERGE!!! Fixes: community.aws/issues/16 This patch ensures that when changing state and/or supported parameters for an instance, changes are applied regardless of the starting or desired state of the instance. This patch depends on Python3 and SHOULD NOT be merged until this module is migrated to the amazon.aws collection and collection version 2.x.
jillr
force-pushed
the
ec2_instance_state_handling
branch
from
585e5e9 to
b4284d8
Compare
ansibullbot
added
bug
| # https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-reboot.html | ||
| # The Ansible behaviour of issuing a stop/start has a minor impact on user billing | ||
| # This will need to be changelogged if we ever change to ec2.reboot_instance | ||
| _changed, _failed, _instances, _failure_reason = change_instance_state( |
There was a problem hiding this comment.
It would also be interesting if this behavior changed because stopping and starting an instance gives new underlying hardware, but a reboot call doesn't. That's an important distinction when there are hardware issues on the instance (if you run long running instances like I do, this is quite a common occurrence).
I hadn't realized that this is what the module did so in playbooks I have for recovering, I do actually call the module twice, once to stop it and once to start. I wonder if anyone has settled on using using the restarted/rebooted statuses in Ansible to recover, only to find that it won't work anymore if this changes. Probably a niche thing...
But it does make me think.. a dedicated integrated "stop then start" mode (what restarted actually does now) would be useful.
There was a problem hiding this comment.
My gut inclination was to change this to actually call `reboot' since it's not really doing what it advertises right now, and we'll have a narrow window before 2.0 where we can make non-backwards-compatible changes. But I don't want to overload this PR with too many changes.
Maybe @tremble can weigh in if it would be useful to make this change?
There was a problem hiding this comment.
I think you're right, if it advertises reboot and a reboot call is available, that's what should be called. I'm only mentioning a case (albeit one that might be rare) where that change could be breaking. And that another option that explicitly does what the current behavior is might have uses. The new option value should be introduced before changing the behavior imo so that there's an overlap where people could make forward-compatible changes if they wanted to keep that behavior.
But I dunno maybe that's all overthinking.
(I also agree it's not really related to this PR)
| # TODO: this breaks check_mode, because change_instance_state handles checkmode but not ensure | ||
| # because ensure passes off to change... |
There was a problem hiding this comment.
That's scary, I hope check mode will work properly before the final version 🙏
There was a problem hiding this comment.
That one might be a lingering comment from an earlier stage of the work-in-progress actually, I'll test to confirm - thanks for catching that!
jillr
changed the title
|
in case anyone else wonders why this has been closed. ec2_instance is being migrated to amazon.aws |
…stance (ansible-collections#551) ec2_ami - Tag the image on creation when creating an image from an instance SUMMARY Tagging an instance during creation avoids the need to make an additional "tag" call on an untagged resource. ISSUE TYPE Feature Pull Request COMPONENT NAME ec2_ami ADDITIONAL INFORMATION fixes: ansible-collections#550 Reviewed-by: Andy Thompson <None> Reviewed-by: Mark Chappell <None> Reviewed-by: Alina Buzachis <None> Reviewed-by: None <None>
SUMMARY
Fixes: #16
This patch ensures that when changing state and/or supported parameters for
an instance, changes are applied regardless of the starting or desired state
of the instance.
This patch depends on Python3 and
SHOULD NOT be merged until this module ismigrated to the amazon.aws collection and collection version 2.x.
We're ready to start working on 2.x now, so this can be merged.
ISSUE TYPE
COMPONENT NAME
ec2_instance
ADDITIONAL INFORMATION
This PR is being opened here WIP to allow the community time to review prior to the module migration to amazon.aws and Python bump to 3-only. A new PR will be opened there and referenced to any reviews here.On further thought, it'll probably be easier to merge this here before migrating the module.Except that I haven't removed the py2.7 tests from CI yet so I'm going to stop changing my mind and get ready to migrate the module!
I'm undecided if I'm happy about adding another integration test suite or if it would make more sense to combine these with the termination_protection tests. Review feedback desired.